Add fastapi arguments attributes directly on the root otel span #509
Conversation
Deploying logfire-docs with
|
| Latest commit: |
cecc093
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://72333100.logfire-docs.pages.dev |
| Branch Preview URL: | https://alex-fastapi-args-root-span.logfire-docs.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 132 132
Lines 10056 10082 +26
Branches 1360 1364 +4
=========================================
+ Hits 10056 10082 +26 ☔ View full report in Codecov by Sentry. |
alexmojaki
requested review from
Kludex and
dmontagu
and removed request for
dmontagu
|
Nice! |
| @@ -58,7 +58,6 @@ def instrument_fastapi( | |||
| dict[str, Any] | None, | |||
| ] | |||
| | None = None, | |||
| use_opentelemetry_instrumentation: bool = True, | |||
There was a problem hiding this comment.
This option was a bad idea on my part, clutters the docs and API, and doesn't really have a purpose any more now that OTEL kwargs are passed through.
|
After chatting with @dmontagu I think it's likely that we won't remove the fastapi args span soon, but this change should be useful anyway. |
|
Ah, this might also fix #533 if it also removes the Separately, I do find it somewhat useful to track the db queries that come from parsing arguments and resolving |
But @JacobHayes if you've instrumented db queries then you'll still see spans for those queries even if we drop the fastapi args span, they just won't all be grouped together. Otherwise you'll see fastapi args taking a long time to resolve but you won't know why. |
Having a separate "FastAPI arguments" span is a pain. It costs extra money and requires writing a join in SQL to combine info about the root span (particularly the duration) with the argument values. I want to get rid of it.
As a first step, this PR takes the same attributes that are set on the fastapi args span and sets them on the root span created by OTEL. This allows people to use the root span in queries and avoid joining in SQL, and should ease the transition to removing the fastapi args span.
In a followup change I will remove the fastapi args span. We might want to treat this as a breaking change and release version 2.0. We might also want to add a parameter (off by default) that keeps the span, although the only reasons I can think of to keep it are quite weak: